Skip to content

Replace BinaryFormatter with PSSerializer #415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Nov 21, 2023
Merged

Replace BinaryFormatter with PSSerializer #415

merged 2 commits into from
Nov 21, 2023

Conversation

andyleejordan
Copy link
Member

@andyleejordan andyleejordan commented Aug 28, 2023 •
edited
Loading

Description

Since the former has been removed from PowerShell 7.4 (because it was removed from .NET 7) and the latter can accomplish the same thing.

Issues Fixed

References

N/A

Checklist

  • You actually ran the code that you just wrote, especially if you did just "one last quick change".
  • Comment-based help added/updated, including examples.
  • Static analysis is reporting back clean.
  • New/changed code adheres to our coding guidelines.
  • Formatters were created for any new types being added.
  • New/changed code continues to support the pipeline.
  • Changes to the manifest file follow the manifest guidance.
  • Unit tests were added/updated and are all passing. See testing guidelines. This includes making sure that all pipeline input variations have been covered.
  • Relevant usage examples have been added/updated in USAGE.md.
  • If desired, ensure your name is added to our Contributors list

@andyleejordan andyleejordan force-pushed the andyleejordan/deepcopy-object branch from f967cb1 to 51abe90 Compare August 28, 2023 19:49
@andyleejordan
Copy link
Member Author

andyleejordan commented Aug 28, 2023 •
edited
Loading

Ah we need to figure out what depth is sufficient:

The input object to serialize. Serializes to a default depth of 1.

...I set it to 64.

@andyleejordan andyleejordan force-pushed the andyleejordan/deepcopy-object branch from 51abe90 to 167761d Compare August 28, 2023 20:25
@andyleejordan
Copy link
Member Author

@HowardWolosky the third commit is a little risker and you may want further testing of it. I verified my previous workflow still worked (and was previously erroring because of the use of BinaryFormatter). @SeeminglyScience pointed out to me that the whole function DeepCopy-Object was unnecessary and could be replaced with a simple addition in the loop to add the converted properties to the new clone object. It's an option, and I've left it in a separate commit if you want it (or you can just take the first two).

@andyleejordan andyleejordan force-pushed the andyleejordan/deepcopy-object branch from d86320b to d152b79 Compare August 28, 2023 20:40
HowardWolosky
HowardWolosky reviewed Aug 28, 2023
View reviewed changes
@andyleejordan andyleejordan force-pushed the andyleejordan/deepcopy-object branch from d152b79 to 167761d Compare August 28, 2023 22:20
andyleejordan
andyleejordan commented Aug 28, 2023
View reviewed changes
HowardWolosky
HowardWolosky reviewed Aug 28, 2023
View reviewed changes
HowardWolosky
HowardWolosky requested changes Aug 28, 2023
View reviewed changes
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for bringing this to my attention with the issue, as well as for providing the fix!

Your change looks good (aside from the minor requested capitalization change) from a code perspective. I just need to run it through some tests to verify that there aren't any unexpected regressions that I'm missing from a cursory review.

@andyleejordan andyleejordan force-pushed the andyleejordan/deepcopy-object branch from 167761d to 8f809fc Compare August 29, 2023 00:10
@andyleejordan
Copy link
Member Author

Thanks so much for bringing this to my attention with the issue, as well as for providing the fix!

Your change looks good (aside from the minor requested capitalization change) from a code perspective. I just need to run it through some tests to verify that there aren't any unexpected regressions that I'm missing from a cursory review.

You're quite welcome! And yes, please verify it further. I use your module in a script to generate a changelog and programmatically create a GitHub release with assets uploaded direct from the CI/CD machines. It's great! I definitely want it to keep working with PowerShell 7.4.

@andyleejordan
Add name to contributors list
44b9fd7
@andyleejordan
Replace BinaryFormatter with PSSerializer
a59bbc2 
Since the former has been removed from PowerShell 7.4 (because it was
removed from .NET 7) and the latter can accomplish the same thing.

The default depth is 1, so we need to specify. Best I could find was
that the prior implementation defaulted to a depth of 64, so we're going
with that.
@andyleejordan andyleejordan force-pushed the andyleejordan/deepcopy-object branch from 8f809fc to a59bbc2 Compare August 29, 2023 19:02
@andyleejordan
Copy link
Member Author

Have you had a chance to test it more thoroughly yet? Thanks!

@andyleejordan
Copy link
Member Author

Hey @HowardWolosky just following up on this as PowerShell 7.4 gets closer to GA (and will result in this module being quite broken without this change). 😄

@brogers5
Copy link

PowerShell 7.4 has since hit GA, and this issue is starting to impact module consumers like myself. Is there anything we can do to help move this PR forward?

HowardWolosky
HowardWolosky approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyleejordan - Thanks so much for getting this change in. My apologies for not completing the testing/validation earlier before PS 7.4 got released. Merging this in now, and then will see if I can get a new module version published.

@HowardWolosky HowardWolosky merged commit 356350b into microsoft:master Nov 21, 2023
@andyleejordan
Copy link
Member Author

Heck yeah, thanks @HowardWolosky!

@andyleejordan andyleejordan deleted the andyleejordan/deepcopy-object branch November 21, 2023 01:28
@HowardWolosky
Copy link
Member

This has been published to PowerShellGallery as part of 0.17.0: https://www.powershellgallery.com/packages/PowerShellForGitHub/0.17.0

@HowardWolosky HowardWolosky mentioned this pull request Feb 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HowardWolosky HowardWolosky HowardWolosky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reliance on BinaryFormatter breaks module with PowerShell 7.4.0-preview.4
3 participants
@andyleejordan @brogers5 @HowardWolosky